Skip to content

feat: BadUrlError #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: BadUrlError #166

wants to merge 1 commit into from
+5 −0

Conversation

srghma
Copy link
Member

@srghma srghma commented Sep 16, 2021 •
edited
Loading

Description of the change

This pr adds BadUrlError, just like in Elm

https://github.com/elm/http/blob/34a9a27411c2492d3e247ac75cd48e22b473bef5/src/Http.elm#L539

which is thrown during xhr.open https://github.com/elm/http/blob/34a9a27411c2492d3e247ac75cd48e22b473bef5/src/Elm/Kernel/Http.js#L33

thought I wasn't been able to reproduce this, instead for me on spago test it was thrown on xhr.send

the error we are handling is (check first commit srghma@0d30f28)

TypeError [ERR_UNESCAPED_CHARACTERS]: Request path contains unescaped characters
    at new NodeError (node:internal/errors:363:5)
    at new ClientRequest (node:_http_client:154:13)
    at Object.request (node:http:96:10)
    at XMLHttpRequest._sendHxxpRequest (/home/srghma/projects/purescript-affjax/node_modules/xhr2/lib/xhr2.js:480:24)
    at XMLHttpRequest._sendHttp (/home/srghma/projects/purescript-affjax/node_modules/xhr2/lib/xhr2.js:460:14)
    at XMLHttpRequest.send (/home/srghma/projects/purescript-affjax/node_modules/xhr2/lib/xhr2.js:278:18)
    at /home/srghma/projects/purescript-affjax/output/Affjax/foreign.js:89:13
    at __do (/home/srghma/projects/purescript-affjax/output/Effect.Aff.Compat/index.js:15:22)
    at runAsync (/home/srghma/projects/purescript-affjax/output/Effect.Aff/foreign.js:98:20)
    at run (/home/srghma/projects/purescript-affjax/output/Effect.Aff/foreign.js:333:22) {
  code: 'ERR_UNESCAPED_CHARACTERS'
}

i'm not sure about this change

Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@srghma srghma changed the title WIP: BadUrlError feat: BadUrlError Oct 10, 2024
@srghma
Copy link
Member Author

srghma commented Oct 10, 2024

TODO: add test to node and web

or maybe check my error code? ERR_UNESCAPED_CHARACTERS

    A.log "Testing invalid url"
    AX.get ResponseFormat.string "/фыва" >>= assertLeft >>= case _ of
       AX.BadUrlError url → assertEq "/фыва" url
       other → logAny' other *> assertFail "Expected a BadUrlError"

@srghma
Copy link
Member Author

srghma commented Oct 10, 2024 

let errorRef = null
function simulateBadUrlError() {
  const XMLHttpRequest = require('xhr2');
  const xhr = new XMLHttpRequest();
  const badUrl = 'http://example.com/фіва';
  xhr.open('GET', badUrl, true);
  xhr.onerror = function (e) { console.error('Request failed', e); errorRef = e; };
  try {
    xhr.send();
  } catch (e) {
    console.error('Caught Error in send():', e); errorRef = e;
  }
}
simulateBadUrlError();

Caught Error in send(): TypeError [ERR_UNESCAPED_CHARACTERS]: Request path contains unescaped characters
    at new ClientRequest (node:_http_client:184:13)
    at Object.request (node:http:102:10)

> errorRef.code
'ERR_UNESCAPED_CHARACTERS'

but returns success in browser

even this doesnt throw our error in browser

let errorRef = null
function simulateBadUrlError() {
  // const XMLHttpRequest = require('xhr2');
  const xhr = new XMLHttpRequest();
  const badUrl = 'http://  фівафіва/фіва';
  xhr.open('GET', badUrl, true);
  xhr.onerror = function (e) { console.error('Request failed', e); errorRef = e; };
  try {
    xhr.send();
  } catch (e) {
    console.error('Caught Error in send():', e); errorRef = e;
  }
}
simulateBadUrlError();

couldnt reproduce with try around open

https://github.com/elm/http/blob/34a9a27411c2492d3e247ac75cd48e22b473bef5/src/Elm/Kernel/Http.js#L30C1-L35C1

but it still makes sence to implement this error

jamesdbrock
jamesdbrock reviewed Oct 18, 2024
View reviewed changes
src/Affjax.purs
@@ -215,6 +218,8 @@ request driver req =
TimeoutError
else if message == requestFailedMessageIdent then
RequestFailedError
else if message == "Request path contains unescaped characters" then
BadUrlError req.url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like RequestContentError would work for this case?

Then we would not have to add BadUrlError and then we would not have to bump the major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesdbrock jamesdbrock jamesdbrock left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@srghma @jamesdbrock